add traits for custom subprocess implementations to aws-types and aws-config#4590
add traits for custom subprocess implementations to aws-types and aws-config#4590codeshaunted wants to merge 4 commits into
aws-types and aws-config#4590Conversation
landonxjames
left a comment
landonxjames
left a comment
There was a problem hiding this comment.
Generally looks reasonable to me. One concern about the newly added feature in aws-config
| credentials-process = ["tokio/process"] | ||
| default = ["default-https-client", "rt-tokio", "credentials-process", "sso"] | ||
| credentials-process = [] | ||
| default-process = ["credentials-process", "tokio/process"] |
There was a problem hiding this comment.
I am hesitant to add a new feature for this functionality. We use cargo hack to test the powerset of feature combinations for this crate. So each new feature we add has a substantial time cost in CI.
Also think this could be a breaking change for users using no-default-features but who were relying on the existing credential process implementation.
There was a problem hiding this comment.
Yeah I think you're right, with that restriction though I'm not sure if there's a way to prevent pulling in tokio/process. I guess we can live with this restriction, I'll update to remove the feature for it soon.
codeshaunted
force-pushed
the
avery/process-provider
branch
from
510a8da to
c65223a
Compare
codeshaunted
force-pushed
the
avery/process-provider
branch
from
c65223a to
cc91574
Compare
|
Thanks for the work on this PR, appreciate the effort. After seeing this PR, stepping back and looking at the bigger picture, we'd like to reconsider the direction here. The goal is to mirror a default credentials provider chain in a virtual environment, as noted in the previous PR
The concern here is that it expands the public trait surface to support virtual environments, and that surface will only grow as more credential sources are added. We'd prefer not to commit to that path. Sorry for the back-and-forth — to make things atomic, would it be ok to revert smithy-rs#4570 (since it introduces public API changes that would ship in the next release) and instead start from detailed problem descriptions and functional requirements? We feel we need to spend more time designing. |
Reverting it is fine, we can just use a fork for now while design is being finalized. I'm open to alternative solutions, just wary about re-implementing the entire default chain, as it seems quite involved. For the design I'm also available for a call to discuss if that would be helpful for you all! |
|
Thank you for your reply. Again, apologize for going back to square one. We'll first create an issue to track the requested functionality of mirroring a default credentials provider chain in a virtual environment. In the issue, it'd be great to provide a high-level description of the problem along with concrete use cases that led to pain points with today's codebase. It not only clarifies the problem space but also serves as useful context for an agent to derive artifacts in the solution space (e.g. functional spec, implementation) |
Per #4590 (comment) [Issue](#4601) has been created to track the desired functionality. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
3 participants
Motivation and Context
After #4570 was merged I realized that to support the full default provider chain with virtualized OS operations we would also need to provide custom implementations for subprocess spawning in
credential-process.Description
Adds a public
ProvideProcesstrait andProcessshim toaws_types::os_shim_internal, following the same pattern asProvideEnv/EnvandProvideFs/Fs. This enables custom implementations of process execution forcredential_process, allowing users to provide custom backends (e.g., sandboxed environments, remote execution, WASM) and enabling tests to mock process execution without spawning real subprocesses.Also updates
ProviderConfigandConfigLoaderinaws_configto accept aProcess, and updatesCredentialProcessProviderto use theProcessabstraction. The default tokio-based process provider is behind the newdefault-processfeature (enabled by default), mirroring thedefault-https-clientpattern. Users can disable it and provide their ownProcessimplementation.Open Questions
Technically this will not be used outside of
aws-config, but I put the interface inos_shim_internalinaws-typesfor consistency withFsandEnv. Should this be moved toaws-configaltogether?Testing
./gradlew :aws:sdk:cargoTestcargo semver-checksinaws/rust-runtimeChecklist
.changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.